This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 660
feat(rome_formatter): Comments multi map #3230
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaReiser
changed the title
Delete JsConstructorParameter kind
feat(rome_formatter): Comments multi map
Sep 15, 2022
MichaReiser
force-pushed
the
refactor/comments
branch
from
September 15, 2022 12:27
4946e49
to
8ce92f8
Compare
This PR implements a multimap for storing the leading, dangling, and trailing comments for nodes. An implementation using a general-purpose implementation would use a multimaps for the leading, trailing, and dangling comments and each multimap would allocate a `Vec` for every node with a comment. This purpose-built multimap uses a shared `Vec` to store all comments where this is possible (99.99% of comments) and only allocates node specific `Vec`s if necessary. The idea behind the implementation is that comments should maintain the same order as in the source document. That means: * All comments for a node are inserted at "the same" time (one at a time, but all together) * It first inserts the leading, then dangling, and finally, a node's trailing comments.
MichaReiser
force-pushed
the
feat/comments-map
branch
from
September 15, 2022 12:27
21e3d2f
to
5fd3664
Compare
ematipico
reviewed
Sep 15, 2022
leading.push(part); | ||
} | ||
|
||
Some(entry) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do we hit this variant? Asking because I read the function entry_to_out_of_order
which does match
against entry
... so I am confused 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is hit if entry
is an Entry::InOrder
and either:
- There are existing dangling comments or trailing comments for this node. It then isn't possible to insert the leading comment because it would require inserting in the middle of the
Vec
which would invalidate the indices and requires copying thedangling
andtrailing
comments - Parts for another key have been inserted in the meantime: So if you have
leading(a, 1), leading(b, 1), leading(a, 2)
(notice thea, b, a
sequence). Here we have the same issue, it is now required to insert the comment in between theleading(a, 1)
andleading(b, 1)
but that requires copying all comments and invalidates all indices.
ematipico
approved these changes
Sep 15, 2022
MichaReiser
added a commit
that referenced
this pull request
Sep 16, 2022
MichaReiser
added a commit
that referenced
this pull request
Sep 22, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR is part of the Comments refactoring (#3227)
This PR implements a multimap for storing the leading, dangling, and trailing comments for nodes.
A naive implementation using three multimaps: one to store the leading, dangling, and trailing parts of each node, requires between
nodes < allocations < nodes * 3
vec
allocations (nodes
= number of nodes with at least one comment).This purpose-built multimap uses a shared
Vec
to store all comments where this is possible (99.99% of comments) and only allocates node-specificVec
s for nodes appearing "out of order"The idea behind the implementation is that comments normally remain in the same order as in the source document. That means:
Using this fact allows storing the vast majority of comments in a single
Vec
and only storing the offsets into this shared vec indicating the start/end of the leading/dangling/trailing comments. The implementation falls back to allocating (up to) three vectors for a node in case either of the above rules is violated.Using a single vector for the majority of comments is also beneficial when it comes to iterating because:
Metrics
I used
countme
to verify that the hypothesis that comments appearing out of order are rare by formatting all benchmark files:Out of the close to 40k comments, only 33 nodes required their "own"
leading
,dangling
, andtrailing
collections. That's 0.0837925%. Or, in other words. This implementation avoids between 40k and 120k vector allocations compared to using three general purpose multimap implementations.Test Plan
I added a handful of tests to the map implementation